Expose User/Org information under site.github.owner #151
Conversation
|
Update: Fixed the test case, so now CI tests pass. I also managed to test this on a project page (by setting env variable I investigated filtering fields such as email (using Thanks! |
filbranden
changed the title
filbranden
changed the title
|
I think we'd feel safer with an allow list. This would also allow us to upgrade to GraphQL in the future. Here's what I think we should allow: {
"login": "octocat",
"id": 1,
"node_id": "MDQ6VXNlcjE=",
"avatar_url": "https://github.com/images/error/octocat_happy.gif",
"html_url": "https://github.com/octocat",
"type": "User",
"site_admin": false,
"name": "monalisa octocat",
"company": "GitHub",
"blog": "https://github.com/blog",
"location": "San Francisco",
"bio": "There once was...",
"public_repos": 2,
"public_gists": 1,
"followers": 20,
"following": 0,
"created_at": "2008-01-14T04:33:35Z",
"updated_at": "2008-01-14T04:33:35Z"
}For organizations, let's allow only the following fields: {
"login": "github",
"id": 1,
"node_id": "MDEyOk9yZ2FuaXphdGlvbjE=",
"url": "https://api.github.com/orgs/github",
"avatar_url": "https://github.com/images/error/octocat_happy.gif",
"description": "A great organization",
"name": "github",
"company": "GitHub",
"blog": "https://github.com/blog",
"location": "San Francisco",
"email": "[email protected]",
"is_verified": true,
"has_organization_projects": true,
"has_repository_projects": true,
"public_repos": 2,
"public_gists": 1,
"followers": 20,
"following": 0,
"html_url": "https://github.com/octocat",
"created_at": "2008-01-14T04:33:35Z",
"type": "Organization",
"collaborators": 8,
}What do you think? |
|
Sure @parkr that list looks pretty reasonable. I can work on an update and push to this PR. Thanks! |
filbranden
force-pushed
the
userinfo1
branch
2 times, most recently
from
a23475c to
d2e5a35
Compare
|
Done. Please take a look. The AppVeyor failures look unrelated... Can you please relaunch those two failing jobs? |
@filbranden Done! |
| :updated_at, | ||
| ]) | ||
|
|
||
| def owner_obj |
There was a problem hiding this comment.
Instead of owner_obj, how about calling this owner_metadata? A comment above the method would also help tremendously! I'm also tempted to move this user lookup & filter into a separate Drop class...
There was a problem hiding this comment.
Done.
(When first writing the code, I was thinking of a follow up that would rename owner to owner_name to make those match... It's a much more intrusive patch, but maybe it would be nice to have the same name on either side?)
In any case, renamed as requested. Please take another look.
|
AppVeyor is 1/4 unhappy... Please restart that build if possible? Thanks! |
|
Thank you so much! @jekyllbot: merge +minor |
|
Awesome, thanks a lot! |
Fixes #149.
Tested locally for a User object, captured everything I was interested in (full name, email, company, bio, ...)
/cc @parkr @benbalter
Thanks!
Filipe